Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[1.21.4] Fix BlockParticleOption#pos not getting sent to the client #1673

Merged
merged 2 commits into from
Dec 16, 2024
Merged

[1.21.4] Fix BlockParticleOption#pos not getting sent to the client #1673

merged 2 commits into from
Dec 16, 2024

Conversation

XFactHD
Copy link
Member

@XFactHD XFactHD commented Nov 9, 2024

This PR fixes the BlockParticleOption#pos field not getting sent to the client, causing custom models with ModelData-sensitive particle texture selection to be unable to fall back to a default value due to receiving ModelData.EMPTY.
To fix this, the StreamCodec of BlockParticleOption is patched to send the position on NeoForge->NeoForge connections.
Vanilla compatibility being retained was tested by connecting with a vanilla client to a NeoForge dev server and falling onto blocks from a sufficient height.

I also opted to slightly clean up the patches around the addition of the position data.

Fixes #1629

@XFactHD XFactHD added bug A bug or error rendering Related to rendering 1.21.3 Targeted at Minecraft 1.21.3 labels Nov 9, 2024
@neoforged-pr-publishing
Copy link

neoforged-pr-publishing bot commented Nov 9, 2024 •
edited
Loading

  • Publish PR to GitHub Packages

Last commit published: 666d93eeb69e92ca200d3ac377bac3bddd0d9ebe.

PR Publishing

The artifacts published by this PR:

Repository Declaration

In order to use the artifacts published by the PR, add the following repository to your buildscript:

repositories {
    maven {
        name 'Maven for PR #1673' // https://github.com/neoforged/NeoForge/pull/1673
        url 'https://prmaven.neoforged.net/NeoForge/pr1673'
        content {
            includeModule('net.neoforged', 'neoforge')
            includeModule('net.neoforged', 'testframework')
        }
    }
}

MDK installation

In order to setup a MDK using the latest PR version, run the following commands in a terminal.
The script works on both *nix and Windows as long as you have the JDK bin folder on the path.
The script will clone the MDK in a folder named NeoForge-pr1673.
On Powershell you will need to remove the -L flag from the curl invocation.

mkdir NeoForge-pr1673
cd NeoForge-pr1673
curl -L https://prmaven.neoforged.net/NeoForge/pr1673/net/neoforged/neoforge/21.4.5-beta-pr-1673-particleoptions_pos_networking/mdk-pr1673.zip -o mdk.zip
jar xf mdk.zip
rm mdk.zip || del mdk.zip

To test a production environment, you can download the installer from here.

@neoforged-compatibility-checks
Copy link

neoforged-compatibility-checks bot commented Nov 9, 2024 •
edited
Loading

@XFactHD, this PR introduces breaking changes.
Fortunately, this project is currently accepting breaking changes, but if they are not intentional, please revert them.
Last checked commit: 666d93eeb69e92ca200d3ac377bac3bddd0d9ebe.

neoforge (:neoforge)

  • net/minecraft/core/particles/BlockParticleOption
    • setPos(Lnet/minecraft/core/BlockPos;)Lnet/minecraft/core/particles/BlockParticleOption;: ❗ API method was removed

@XFactHD XFactHD changed the title [1.21.3] Fix BlockParticleOption#pos not getting sent to the client [1.21.4] Fix BlockParticleOption#pos not getting sent to the client Dec 4, 2024
sciwhiz12
sciwhiz12 reviewed Dec 10, 2024
View reviewed changes
Copy link
Member

@sciwhiz12 sciwhiz12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tested running this across two different NeoForge versions, one with the fix and one without? I'm a bit dubious on the outcome if one side expects the position data to be there, and the other does not.

@XFactHD
Copy link
Member Author

XFactHD commented Dec 10, 2024

If one side is patched and the other is not then this will blow up. That's why I won't backport this fix to 21.1.

@sciwhiz12 sciwhiz12 added 1.21.4 Targeted at Minecraft 1.21.4 breaking change Breaks binary compatibility and removed 1.21.3 Targeted at Minecraft 1.21.3 labels Dec 10, 2024
@Technici4n Technici4n merged commit 709add9 into neoforged:1.21.x Dec 16, 2024
6 checks passed
@neoforged-releases
Copy link

🚀 This PR has been released as NeoForge version 21.4.28-beta.

@neoforged-releases neoforged-releases bot mentioned this pull request Dec 16, 2024
Closed
@XFactHD XFactHD deleted the particleoptions_pos_networking branch December 16, 2024 21:37
@Gaming32 Gaming32 mentioned this pull request Dec 17, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sciwhiz12 sciwhiz12 sciwhiz12 left review comments

@embeddedt embeddedt embeddedt approved these changes

Assignees
No one assigned
Labels
1.21.4 Targeted at Minecraft 1.21.4 breaking change Breaks binary compatibility bug A bug or error rendering Related to rendering
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

BlockParticleOption#pos is not serialized and end up breaking blocks that relay on ModelData for particles
4 participants
@XFactHD @sciwhiz12 @embeddedt @Technici4n